feat: use archive nodes to query attestations #571
Conversation
…ng' into use-historical-data-when-deploying
Codecov Report
@@ Coverage Diff @@
## main #571 +/- ##
==========================================
- Coverage 34.84% 32.78% -2.06%
==========================================
Files 27 27
Lines 2230 2370 +140
==========================================
Hits 777 777
- Misses 1360 1500 +140
Partials 93 93
|
|
@coderabbitai review |
|
Warning Rate Limit Exceeded@sweexordious has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 12 minutes and 38 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per repository. WalkthroughThe changes in this update focus on enhancing the querying capabilities of the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (7)
- cmd/blobstream/deploy/cmd.go (3 hunks)
- cmd/blobstream/deploy/config.go (5 hunks)
- cmd/blobstream/deploy/errors.go (1 hunks)
- e2e/scripts/deploy_blobstream_contract.sh (1 hunks)
- relayer/relayer.go (1 hunks)
- rpc/app_querier.go (7 hunks)
- rpc/errors.go (1 hunks)
Files skipped from review due to trivial changes (2)
- cmd/blobstream/deploy/errors.go
- rpc/errors.go
Additional comments: 12
relayer/relayer.go (1)
- 148-161: The fallback mechanism for querying the last valset before a nonce is well implemented. It first tries to recover by querying the valset from the P2P network. If that fails, it falls back to using an archive node to query the valset. The function now returns an error if both attempts to query the valset fail. This is a good practice for error handling and recovery.
cmd/blobstream/deploy/config.go (5)
17-24: The new flags
FlagCoreRPCHostandFlagCoreRPCPortare introduced correctly.28-32: The new flags are added to the command with default values. Ensure that these default values are appropriate for your use case.
52-61: The
deployConfigstruct is updated to include thecoreRPCfield. This change is consistent with the new flags introduced.80-93: The new flags are parsed correctly and error handling is in place.
120-123: The
coreRPCfield is populated using the parsed values of the new flags. The format of thecoreRPCstring seems to be correct.e2e/scripts/deploy_blobstream_contract.sh (1)
- 68-72: The addition of the
--core.rpc.hostand--core.rpc.portflags to the deployment command is consistent with the PR summary. Ensure that these flags are correctly handled in theblobstream deploycommand and that the RPC connection is properly established and used.cmd/blobstream/deploy/cmd.go (1)
- 142-170: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [146-198]
The
getStartingValsetfunction has been updated to handle different cases for thestartingNonceargument. The error handling and fallback logic are well implemented. However, the function could be simplified by extracting the repeated code into a separate function.rpc/app_querier.go (4)
22-22: The global variable
BlocksIn20DaysPeriodis calculated based onappconsts.TimeoutCommit.Seconds(). Ensure thatappconsts.TimeoutCommit.Seconds()is always a constant and not subject to change during runtime. If it can change, consider moving this calculation into a function to ensure the value is always accurate.73-96: The function
QueryHistoricalAttestationByNoncequeries an attestation by nonce from the state machine at a certain height. It uses thegrpc.Headerfunction to retrieve the header from the response. Ensure that the header is used correctly in the rest of the code.139-154: The function
QueryHistoricalLatestAttestationNoncequeries the historical latest attestation nonce from the state machine at a certain nonce. It uses thegrpc.Headerfunction to retrieve the header from the response. Ensure that the header is used correctly in the rest of the code.212-227: The function
QueryHistoricalValsetByNoncequeries a historical valset by nonce. It uses theQueryHistoricalAttestationByNoncefunction to get the attestation and then checks if it's a valset. Ensure that all attestations that are valsets can be correctly unmarshalled into theValsettype.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- cmd/blobstream/deploy/cmd.go (3 hunks)
- rpc/app_querier.go (7 hunks)
Additional comments: 15
cmd/blobstream/deploy/cmd.go (6)
43-56: The
appQuerierinstance is started and stopped correctly. The error handling and logging are also done properly.58-68: The
tmQuerierinstance is started and stopped correctly. The error handling and logging are also done properly.70-75: The error handling and logging in the
getStartingValsetfunction call are done correctly.146-156: The
getStartingValsetfunction has been updated to handle errors when querying the latest valset. If an error occurs, it attempts to query historical data. This is a good fallback mechanism.160-168: The function also handles errors when querying attestations by nonce. If an error occurs, it attempts to query historical data. This is a good fallback mechanism.
176-195: The function handles errors when parsing the starting nonce and querying historical attestations by nonce. If the attestation is of type
types.Valset, it returns the value. If it's of typetypes.DataCommitment, it queries the last valset before the nonce. This is a good way to handle different types of attestations.rpc/app_querier.go (9)
- 2-13: The import statements are well organized and there are no unused imports. The import paths are correct.
23:
The global variableBlocksIn20DaysPeriodis calculated correctly. However, it's better to add a comment to explain what it represents and why it's calculated this way.
25-27: The
AppQuerierstruct is well defined with appropriate fields. The comments provide a clear explanation of its purpose.74-97: The
QueryHistoricalAttestationByNoncefunction is correctly implemented. It queries an attestation by nonce from the state machine at a certain height. The error handling is also done correctly.99-130: The
QueryRecursiveHistoricalAttestationByNoncefunction is correctly implemented. It queries an attestation by nonce from the state machine by going over the history step by step starting from a certain height. The error handling and the use of context with timeout are also done correctly.147-162: The
QueryHistoricalLatestAttestationNoncefunction is correctly implemented. It queries the historical latest attestation nonce from the state machine at a certain height. The error handling is also done correctly.220-236: The
QueryHistoricalValsetByNoncefunction is correctly implemented. It queries a historical valset by nonce. The error handling is also done correctly.257-286: The
QueryRecursiveLatestValsetfunction is correctly implemented. It queries the latest recorded valset in the state machine in history. The error handling and the use of context with timeout are also done correctly.305-319: The
QueryHistoricalLastValsetBeforeNoncefunction is correctly implemented. It returns the last historical valset before nonce for a certain height. The error handling is also done correctly.321-347: The
QueryRecursiveHistoricalLastValsetBeforeNoncefunction is correctly implemented. It recursively looks for the last historical valset before nonce for a certain height until genesis. The error handling and the use of context with timeout are also done correctly.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (7)
- orchestrator/orchestrator_test.go (1 hunks)
- orchestrator/suite_test.go (2 hunks)
- relayer/relayer_test.go (1 hunks)
- rpc/app_historic_querier_test.go (1 hunks)
- rpc/app_querier.go (7 hunks)
- rpc/historic_suite_test.go (1 hunks)
- testing/celestia_network.go (2 hunks)
Additional comments: 25
relayer/relayer_test.go (1)
- 68-74: The
Pruningfield is set to "default" andTimeoutCommitis set to 5 milliseconds. Ensure that these settings are appropriate for your use case. TheTimeoutCommitvalue seems quite low, which could lead to frequent timeouts if the system is under heavy load. Consider increasing this value if you experience issues.orchestrator/orchestrator_test.go (1)
- 206-212: The test configuration has been updated to include a
Pruningfield with the value "default" and theTimeoutCommitfield has been modified to have a value of 5 milliseconds. Ensure that these changes are compatible with the rest of the test suite and the application as a whole.orchestrator/suite_test.go (2)
3-9: The "time" package has been added to the import list. Ensure that it is used in the codebase.
35-37: The "TimeIotaMs" field value has been set to 1, the "Pruning" field has been added with the value "default", and the "TimeoutCommit" field has been added with the value 5 * time.Millisecond. Ensure that these changes are compatible with the rest of the codebase.
testing/celestia_network.go (3)
42-48: The
CelestiaNetworkParamsstruct has been extended with two new fields:PruningandTimeoutCommit. Ensure that these fields are properly used and initialized throughout the codebase.51-57: The
DefaultCelestiaNetworkParamsfunction has been updated to set default values for the new fields. Ensure that these default values are appropriate for your use case.79-84: The
NewCelestiaNetworkfunction has been updated to use the newparams.Pruningandparams.TimeoutCommitvalues. This is a good use of the new parameters, as it allows for more flexible configuration of theCelestiaNetwork.rpc/historic_suite_test.go (3)
1-20: The import statements are well organized and separated by functionality. Good use of aliasing to avoid naming conflicts.
22-27: The
HistoricQuerierTestSuitestruct is well defined with all necessary fields for the test suite.49-51: The
TestHistoricQueriersfunction is correctly defined and runs the test suite as expected.rpc/app_querier.go (15)
2-13: The import section looks fine. All the imported packages are being used in the code.
20-27: The global variable
BlocksIn20DaysPeriodis defined and theAppQuerierstruct is declared. The struct has fields for the RPC address, a gRPC client connection, a logger, and an encoding configuration.71-72: The
QueryAttestationByNoncefunction is correctly implemented. It queries an attestation by nonce from the state machine and returns it.74-97: The
QueryHistoricalAttestationByNoncefunction is correctly implemented. It queries an attestation by nonce from the state machine at a certain height and returns it.99-133: The
QueryRecursiveHistoricalAttestationByNoncefunction is correctly implemented. It queries an attestation by nonce from the state machine by going over the history step by step starting from a certain height and returns it.71-137: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [135-147]
The
QueryLatestAttestationNoncefunction is correctly implemented. It queries the latest attestation nonce from the state machine and returns it.
150-164: The
QueryHistoricalLatestAttestationNoncefunction is correctly implemented. It queries the historical latest attestation nonce from the state machine at a certain height and returns it.220-221: The
QueryValsetByNoncefunction is correctly implemented. It queries a valset by nonce and returns it.223-238: The
QueryHistoricalValsetByNoncefunction is correctly implemented. It queries a historical valset by nonce and returns it.257-258: The
QueryLatestValsetfunction is correctly implemented. It queries the latest recorded valset in the state machine and returns it.260-291: The
QueryRecursiveLatestValsetfunction is correctly implemented. It queries the latest recorded valset in the state machine in history and returns it.308-309: The
QueryLastValsetBeforeNoncefunction is correctly implemented. It returns the last valset before a certain nonce.311-324: The
QueryHistoricalLastValsetBeforeNoncefunction is correctly implemented. It returns the last historical valset before a certain nonce for a certain height.327-355: The
QueryRecursiveHistoricalLastValsetBeforeNoncefunction is correctly implemented. It recursively looks for the last historical valset before a certain nonce for a certain height until genesis and returns it.358-360: The
QueryLastUnbondingHeightfunction is correctly implemented. It queries the last unbonding height from the state machine and returns it.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- rpc/app_historic_querier_test.go (1 hunks)
- rpc/historic_suite_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- rpc/app_historic_querier_test.go
Additional comments: 5
rpc/historic_suite_test.go (5)
1-20: Imports look good, no unused or missing imports detected.
22-27: The
HistoricQuerierTestSuitestruct is well defined with necessary fields.29-47: The
SetupSuitefunction is correctly setting up the test suite. It's good to see that error handling is in place forWaitForHeightWithTimeout.49-51: The test suite is correctly run using
suite.Run.53-64: The
setupAppQuerierfunction correctly sets up and starts anAppQuerierinstance. It's good to see that cleanup is being handled usings.T().Cleanup.
There was a problem hiding this comment.
do all of these need to be expoesed by the api? also can we not use latest and last? similar to our other previous discussions around this I think it would be good to just use Last and earliest.
QueryHistoricalAttestationByNonce, QueryRecursiveHistoricalAttestationByNonce, QueryHistoricalLatestAttestationNonce, QueryHistoricalValsetByNonce, QueryRecursiveLatestValset, QueryHistoricalLastValsetBeforeNonce, QueryRecursiveHistoricalLastValsetBeforeNonce.
these names are very confusing imo
| func (aq *AppQuerier) QueryHistoricalLastValsetBeforeNonce(ctx context.Context, nonce uint64, height uint64) (*celestiatypes.Valset, error) { | ||
| queryClient := celestiatypes.NewQueryClient(aq.clientConn) | ||
| var header metadata.MD | ||
| resp, err := queryClient.LatestValsetRequestBeforeNonce( | ||
| metadata.AppendToOutgoingContext(ctx, cosmosgrpc.GRPCBlockHeightHeader, strconv.FormatUint(height, 10)), | ||
| &celestiatypes.QueryLatestValsetRequestBeforeNonceRequest{Nonce: nonce}, | ||
| grpc.Header(&header), | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return resp.Valset, nil | ||
| } | ||
|
|
||
| // QueryRecursiveHistoricalLastValsetBeforeNonce recursively looks for the last historical valset before nonce for a certain height until genesis. | ||
| func (aq *AppQuerier) QueryRecursiveHistoricalLastValsetBeforeNonce(ctx context.Context, nonce uint64, height uint64) (*celestiatypes.Valset, error) { |
There was a problem hiding this comment.
are both of these used independently, or do we only ever call one of these?
There was a problem hiding this comment.
One of them queries at an exact height, the other goes range by range of blocks to find the attestation. So yes, both is used
|
I prefer to do these renames:
in a separate PR. Since that would require renaming different parts of the implementation and I don't want to include all of that in this PR. |
|
@evan-forbes What do you think about:
? |
There was a problem hiding this comment.
the api increase isn't that big of deal since its just the orchestrator/relayer, and might require a larger PR to fix properly. This is just something that was sticking was all. Ideally I think it would be great to isolate the actual queries made to the app in the app querier, and then refactor and separate the logic for getting the last, earliest, before nonce / after nonce
it would be nice to be consistent with last and earliest like we are doing in the app, as I often get confused since I'm not 100% sure we're using latest consistently. I could also just not be understanding latest.
…ng' into use-historical-data-when-deploying
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- relayer/historic_relayer_test.go (1 hunks)
- relayer/historic_suite_test.go (1 hunks)
- rpc/app_querier.go (7 hunks)
Additional comments: 6
relayer/historic_suite_test.go (1)
- 1-66: The test suite and test case setup look good. The
HistoricalRelayerTestSuiteis well-structured and theSetupSuiteandTearDownSuitemethods are correctly implemented. TheTestHistoricRelayerfunction correctly runs the test suite. However, ensure that theHistoricalRelayerTestSuiteis being used in the right context and that the test cases within it are correctly testing the functionality they are supposed to.rpc/app_querier.go (5)
2-13: The new imports seem to be necessary for the new functions and types used in this file. Ensure that all these packages are included in your dependencies.
71-133: The new functions
QueryHistoricalAttestationByNonceandQueryRecursiveHistoricalAttestationByNonceare well implemented. They use the gRPC client to query attestations by nonce at a specific height or recursively. The error handling and context cancellation are correctly implemented.257-296: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [147-292]
The new functions
QueryHistoricalLatestAttestationNonce,QueryRecursiveLatestValset,QueryHistoricalLastValsetBeforeNonce, andQueryRecursiveHistoricalLastValsetBeforeNonceare well implemented. They use the gRPC client to query attestations and valsets at a specific height or recursively. The error handling and context cancellation are correctly implemented.
308-349: The new function
QueryRecursiveHistoricalLastValsetBeforeNonceis well implemented. It uses the gRPC client to query valsets at a specific height or recursively. The error handling and context cancellation are correctly implemented.351-353: The new function
QueryLastUnbondingHeightis well implemented. It uses the gRPC client to query the last unbonding height from the state machine. The error handling is correctly implemented.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- relayer/historic_relayer_test.go (1 hunks)
- rpc/app_querier.go (7 hunks)
Files skipped from review due to trivial changes (1)
- relayer/historic_relayer_test.go
Additional comments: 12
rpc/app_querier.go (12)
2-13: The import statements are well organized and there are no unused imports.
20-25: The global variable
BlocksIn20DaysPeriodis well defined and the comment explains its purpose clearly.27-29: The
AppQuerierstruct is well defined with appropriate fields.76-98: The
QueryHistoricalAttestationByNoncefunction is well implemented with proper error handling. It queries an attestation by nonce from the state machine at a certain height.101-135: The
QueryRecursiveHistoricalAttestationByNoncefunction is well implemented with proper error handling and a timeout context. It queries an attestation by nonce from the state machine by going over the history step by step starting from a certain height.152-166: The
QueryHistoricalLatestAttestationNoncefunction is well implemented with proper error handling. It queries the historical latest attestation nonce from the state machine at a certain height.169-171: The
QueryDataCommitmentByNoncefunction is well implemented with proper error handling. It queries a data commitment by its nonce.225-240: The
QueryHistoricalValsetByNoncefunction is well implemented with proper error handling. It queries a historical valset by nonce.262-293: The
QueryRecursiveLatestValsetfunction is well implemented with proper error handling and a timeout context. It queries the latest recorded valset in the state machine in history.313-326: The
QueryHistoricalLastValsetBeforeNoncefunction is well implemented with proper error handling. It returns the last historical valset before nonce for a certain height.329-351: The
QueryRecursiveHistoricalLastValsetBeforeNoncefunction is well implemented with proper error handling and a timeout context. It recursively looks for the last historical valset before nonce for a certain height until genesis.353-355: The
QueryLastUnbondingHeightfunction is well implemented with proper error handling. It queries the last unbonding height from the state machine.
|
@evan-forbes is ready for review again, only added a relayer test with an archive node. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- relayer/relayer.go (1 hunks)
- rpc/app_querier.go (7 hunks)
Additional comments: 15
rpc/app_querier.go (11)
2-13: The import statements are well organized and there are no unused imports.
20-25: The global variable
BlocksIn20DaysPeriodis well defined and the comment explains its purpose clearly.27-29: The
AppQuerierstruct is well defined with appropriate fields.76-98: The
QueryHistoricalAttestationByNoncefunction is well implemented with proper error handling. It queries an attestation by nonce from the state machine at a certain height.101-134: The
QueryRecursiveHistoricalAttestationByNoncefunction is well implemented with proper error handling and a timeout context. It queries an attestation by nonce from the state machine by going over the history step by step starting from height.151-165: The
QueryHistoricalLatestAttestationNoncefunction is well implemented with proper error handling. It queries the historical latest attestation nonce from the state machine at a certain nonce.224-239: The
QueryHistoricalValsetByNoncefunction is well implemented with proper error handling. It queries a historical valset by nonce.261-291: The
QueryRecursiveLatestValsetfunction is well implemented with proper error handling and a timeout context. It queries the latest recorded valset in the state machine in history.311-324: The
QueryHistoricalLastValsetBeforeNoncefunction is well implemented with proper error handling. It returns the last historical valset before nonce for a certain height.327-351: The
QueryRecursiveHistoricalLastValsetBeforeNoncefunction is well implemented with proper error handling and a timeout context. It recursively looks for the last historical valset before nonce for a certain height until genesis.353-355: The
QueryLastUnbondingHeightfunction is well implemented with proper error handling. It queries the last unbonding height from state machine.relayer/relayer.go (4)
145-162: The error handling and fallback logic for retrieving the previous valset is well implemented. It first tries to get the valset from the app querier, then falls back to the P2P network, and finally tries to get it from an archive node. This ensures that the function can still retrieve the valset even if some sources fail. However, it's important to note that querying from an archive node can be slow and resource-intensive, so it should be used as a last resort.
163-164: The type switch statement is a good way to handle different types of attestations. It ensures that the correct processing logic is used for each type.
145-164: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [165-211]
The logic for processing the attestations is well implemented. It retrieves the necessary data, validates it, and then submits it to the EVM client. However, it's important to ensure that the EVM client is properly handling these transactions and that any errors are being correctly propagated back to this function.
- 145-164: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [212-214]
The default case in the type switch statement correctly handles unknown attestation types by returning an error. This is a good practice as it ensures that the function fails fast when it encounters an unexpected input.
rach-id
changed the title
Overview
Supports querying for historical data when getting attestations that are pruned.
Would allow deploying the contract and also relay attestations if the corresponding valset got pruned via getting it from an archive node.
Checklist
Summary by CodeRabbit
New Features
ProcessAttestationfunction to recover from failed valset queries.FlagCoreRPCHostandFlagCoreRPCPortto specify the RPC address host and port.Refactor
queriertoappQuerierfor clarity.getStartingValsetfunction to use thetmQuerierandappQuerierinstances.Tests
Bug Fixes
getStartingValsetto handle cases where the valset cannot be retrieved from the node state.